[HLSL][RootSignature] Allow for multiple parsing errors in `RootSignatureParser` #147832

inbelic · 2025-07-09T21:31:03Z

This pr implements returning multiple parsing errors at the granularity of a RootElement

This is achieved by adding a new interface onto RootSignatureParser, namely, skipUntilExpectedToken. This will be used to consume all the intermediate tokens between when an error has occurred and when the next RootElement begins.

At this granularity, the implementation is somewhat straight forward, as we can just implement this skip function when we return from a parse[RootElement] method and continue in the main parse loop. With the exception that the parseDescriptorTable will also have to skip ahead to the next expected closing ')'.

If we want to provide any finer granularity, then the skip logic becomes significantly more complicated. Skipping to the next root element will provide a good ratio of user experience benefit to complexity of implementation.

For more context see linked issue.

Updates HLSLRootSignatureParser with a skipUntilExpectedToken interface
Updates the parse loops to use the skip interface when an error is found on parsing a root element
Updates parseDescriptorTable to skip ahead to the next ')' if it was inside a clause
Adds test-case to demonstrate multiple error being reported

Resolves: #145818

llvmbot · 2025-07-09T21:31:45Z

@llvm/pr-subscribers-hlsl

@llvm/pr-subscribers-clang

Author: Finn Plummer (inbelic)

Changes

This pr implements returning multiple parsing errors at the granularity of a RootElement

This is achieved by adding a new interface onto RootSignatureParser, namely, skipUntilExpectedToken. This will be used to consume all the intermediate tokens between when an error has occurred and when the next RootElement begins.

At this granularity, the implementation is quite straight forward, as we can just implement this skip function when we return from a parse[RootElement] method and continue in the main parse loop. Then continue the loop.

If we want to provide any finer granularity, then the skip logic becomes significantly more complicated. Skipping to the next root element will provide a good ratio of user experience benefit to complexity of implementation.

For more context see linked issue.

Updates HLSLRootSignatureParser with a skipUntilExpectedToken interface
Updates the parse loops to use the skip interface when an error is found on parsing a root element
Adds test-case to demonstrate multiple error being reported

Resolves: #145818

Full diff: https://github.com/llvm/llvm-project/pull/147832.diff

3 Files Affected:

(modified) clang/include/clang/Parse/ParseHLSLRootSignature.h (+7)
(modified) clang/lib/Parse/ParseHLSLRootSignature.cpp (+52-19)
(modified) clang/test/SemaHLSL/RootSignature-err.hlsl (+22)

diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 404bccea10c99..09602602224e9 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -198,6 +198,13 @@ class RootSignatureParser {
   bool tryConsumeExpectedToken(RootSignatureToken::Kind Expected);
   bool tryConsumeExpectedToken(ArrayRef<RootSignatureToken::Kind> Expected);
 
+  /// Consume tokens until the expected token has been peeked to be next
+  /// or we have reached the end of the stream. Note that this means the
+  /// expected token will be the next token not CurToken.
+  ///
+  /// Returns true if it found a token of the given type.
+  bool skipUntilExpectedToken(ArrayRef<RootSignatureToken::Kind> Expected);
+
   /// Convert the token's offset in the signature string to its SourceLocation
   ///
   /// This allows to currently retrieve the location for multi-token
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index 87de627a98fb7..8d0847fb770e3 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -17,6 +17,15 @@ namespace hlsl {
 
 using TokenKind = RootSignatureToken::Kind;
 
+static const TokenKind RootElementKeywords[] = {
+    TokenKind::kw_RootFlags,
+    TokenKind::kw_CBV,
+    TokenKind::kw_UAV,
+    TokenKind::kw_SRV,
+    TokenKind::kw_DescriptorTable,
+    TokenKind::kw_StaticSampler,
+};
+
 RootSignatureParser::RootSignatureParser(
     llvm::dxbc::RootSignatureVersion Version,
     SmallVector<RootSignatureElement> &Elements, StringLiteral *Signature,
@@ -27,51 +36,63 @@ RootSignatureParser::RootSignatureParser(
 bool RootSignatureParser::parse() {
   // Iterate as many RootSignatureElements as possible, until we hit the
   // end of the stream
+  bool HadError = false;
   while (!peekExpectedToken(TokenKind::end_of_stream)) {
+    bool HadLocalError = false;
     if (tryConsumeExpectedToken(TokenKind::kw_RootFlags)) {
       SourceLocation ElementLoc = getTokenLocation(CurToken);
       auto Flags = parseRootFlags();
-      if (!Flags.has_value())
-        return true;
-      Elements.emplace_back(RootSignatureElement(ElementLoc, *Flags));
+      if (Flags.has_value())
+        Elements.emplace_back(RootSignatureElement(ElementLoc, *Flags));
+      else
+        HadLocalError = true;
     } else if (tryConsumeExpectedToken(TokenKind::kw_RootConstants)) {
       SourceLocation ElementLoc = getTokenLocation(CurToken);
       auto Constants = parseRootConstants();
-      if (!Constants.has_value())
-        return true;
-      Elements.emplace_back(RootSignatureElement(ElementLoc, *Constants));
+      if (Constants.has_value())
+        Elements.emplace_back(RootSignatureElement(ElementLoc, *Constants));
+      else
+        HadLocalError = true;
     } else if (tryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) {
       SourceLocation ElementLoc = getTokenLocation(CurToken);
       auto Table = parseDescriptorTable();
-      if (!Table.has_value())
-        return true;
-      Elements.emplace_back(RootSignatureElement(ElementLoc, *Table));
+      if (Table.has_value())
+        Elements.emplace_back(RootSignatureElement(ElementLoc, *Table));
+      else
+        HadLocalError = true;
     } else if (tryConsumeExpectedToken(
                    {TokenKind::kw_CBV, TokenKind::kw_SRV, TokenKind::kw_UAV})) {
       SourceLocation ElementLoc = getTokenLocation(CurToken);
       auto Descriptor = parseRootDescriptor();
-      if (!Descriptor.has_value())
-        return true;
-      Elements.emplace_back(RootSignatureElement(ElementLoc, *Descriptor));
+      if (Descriptor.has_value())
+        Elements.emplace_back(RootSignatureElement(ElementLoc, *Descriptor));
+      else
+        HadLocalError = true;
     } else if (tryConsumeExpectedToken(TokenKind::kw_StaticSampler)) {
       SourceLocation ElementLoc = getTokenLocation(CurToken);
       auto Sampler = parseStaticSampler();
-      if (!Sampler.has_value())
-        return true;
-      Elements.emplace_back(RootSignatureElement(ElementLoc, *Sampler));
+      if (Sampler.has_value())
+        Elements.emplace_back(RootSignatureElement(ElementLoc, *Sampler));
+      else
+        HadLocalError = true;
     } else {
+      HadLocalError = true;
       consumeNextToken(); // let diagnostic be at the start of invalid token
       reportDiag(diag::err_hlsl_invalid_token)
           << /*parameter=*/0 << /*param of*/ TokenKind::kw_RootSignature;
-      return true;
     }
 
-    // ',' denotes another element, otherwise, expected to be at end of stream
-    if (!tryConsumeExpectedToken(TokenKind::pu_comma))
+    if (HadLocalError) {
+      HadError = true;
+      skipUntilExpectedToken(RootElementKeywords);
+    } else if (!tryConsumeExpectedToken(TokenKind::pu_comma)) {
+      // ',' denotes another element, otherwise, expected to be at end of stream
       break;
+    }
   }
 
-  return consumeExpectedToken(TokenKind::end_of_stream,
+  return HadError ||
+         consumeExpectedToken(TokenKind::end_of_stream,
                               diag::err_expected_either, TokenKind::pu_comma);
 }
 
@@ -1371,6 +1392,18 @@ bool RootSignatureParser::tryConsumeExpectedToken(
   return true;
 }
 
+bool RootSignatureParser::skipUntilExpectedToken(
+    ArrayRef<TokenKind> AnyExpected) {
+
+  while (!peekExpectedToken(AnyExpected)) {
+    if (peekExpectedToken(TokenKind::end_of_stream))
+      return false;
+    consumeNextToken();
+  }
+
+  return true;
+}
+
 SourceLocation RootSignatureParser::getTokenLocation(RootSignatureToken Tok) {
   return Signature->getLocationOfByte(Tok.LocOffset, PP.getSourceManager(),
                                       PP.getLangOpts(), PP.getTargetInfo());
diff --git a/clang/test/SemaHLSL/RootSignature-err.hlsl b/clang/test/SemaHLSL/RootSignature-err.hlsl
index b69807132e795..439dc265645f8 100644
--- a/clang/test/SemaHLSL/RootSignature-err.hlsl
+++ b/clang/test/SemaHLSL/RootSignature-err.hlsl
@@ -103,3 +103,25 @@ void bad_root_signature_22() {}
 // expected-error@+1 {{invalid value of RootFlags}}
 [RootSignature("RootFlags(local_root_signature | root_flag_typo)")]
 void bad_root_signature_23() {}
+
+#define DemoMultipleErrorsRootSignature \
+  "CBV(b0, invalid)," \
+  "StaticSampler()" \
+  "DescriptorTable(" \
+  "  visibility = SHADER_VISIBILITY_ALL," \
+  "  visibility = SHADER_VISIBILITY_DOMAIN," \
+  ")," \
+  "SRV(t0, space = 28947298374912374098172)" \
+  "UAV(u0, flags = 3)" \
+  "DescriptorTable(Sampler(s0 flags = DATA_VOLATILE))," \
+  "CBV(b0),,"
+
+// expected-error@+7 {{invalid parameter of CBV}}
+// expected-error@+6 {{did not specify mandatory parameter 's register'}}
+// expected-error@+5 {{specified the same parameter 'visibility' multiple times}}
+// expected-error@+4 {{integer literal is too large to be represented as a 32-bit signed integer type}}
+// expected-error@+3 {{flag value is neither a literal 0 nor a named value}}
+// expected-error@+2 {{expected ')' or ','}}
+// expected-error@+1 {{invalid parameter of RootSignature}}
+[RootSignature(DemoMultipleErrorsRootSignature)]
+void multiple_errors() {}

spall · 2025-07-11T22:10:07Z

clang/lib/Parse/ParseHLSLRootSignature.cpp

+        HadLocalError = true;
+        // We are within a DescriptorTable, we will do our best to recover
+        // by skipping until we encounter the expected closing ')'.
+        skipUntilExpectedToken(TokenKind::pu_r_paren);


why do you do this here and not in the other cases? (i'm lacking in knowledge)

This is only done on the descriptor table because we want to avoid accidently parsing an element of the descriptor table as a root element. So we need to "exit" the descriptor table scope. For instance:

DescriptorTable( SRV(s0, invalid), CBV(b0, numDescriptor = 2) ), UAV(u0)

Parsing would fail here on the invalid token. We don't want to keep parsing the CBV as a root element, but we do want to try again starting at UAV. This is done because they have the same token.

Typing this out, I realized though that we actually need to skip until it is closed, not just to the next one.

Updated for this and added a test-case.

clang/lib/Parse/ParseHLSLRootSignature.cpp

V-FEXrt · 2025-07-11T22:27:04Z

clang/lib/Parse/ParseHLSLRootSignature.cpp

+      if (Constants.has_value())
+        Elements.emplace_back(RootSignatureElement(ElementLoc, *Constants));
+      else
+        HadLocalError = true;


bools inside of a loop like this often signal extra complexity that can be removed with a bit of refactoring and this loop certainly has some complexity.

I'm wondering if there is a simpler option. Maybe something like the following

bool RootSignatureParser::parse() { while (!peekExpectedToken(TokenKind::end_of_stream)) { if (tryConsumeExpectedToken(TokenKind::kw_RootFlags)) { SourceLocation ElementLoc = getTokenLocation(CurToken); auto Flags = parseRootFlags(); if (!Flags) { HadError = true; skipUntilExpectedToken(RootElementKeywords); continue; } Elements.emplace_back(RootSignatureElement(ElementLoc, *Flags)); } else if (tryConsumeExpectedToken(TokenKind::kw_RootConstants)) { SourceLocation ElementLoc = getTokenLocation(CurToken); auto Constants = parseRootConstants(); if (!Constants) { HadError = true; skipUntilExpectedToken(RootElementKeywords); continue; } Elements.emplace_back(RootSignatureElement(ElementLoc, *Constants)); } else if (tryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) { SourceLocation ElementLoc = getTokenLocation(CurToken); auto Table = parseDescriptorTable(); if (!Table) { // We are within a DescriptorTable, we will do our best to recover // by skipping until we encounter the expected closing ')'. skipUntilExpectedToken(TokenKind::pu_r_paren); consumeNextToken(); HadError = true; skipUntilExpectedToken(RootElementKeywords); continue; } Elements.emplace_back(RootSignatureElement(ElementLoc, *Table)); } else if (tryConsumeExpectedToken( {TokenKind::kw_CBV, TokenKind::kw_SRV, TokenKind::kw_UAV})) { SourceLocation ElementLoc = getTokenLocation(CurToken); auto Descriptor = parseRootDescriptor(); if (!Descriptor) { HadError = true; skipUntilExpectedToken(RootElementKeywords); continue; } Elements.emplace_back(RootSignatureElement(ElementLoc, *Descriptor)); } else if (tryConsumeExpectedToken(TokenKind::kw_StaticSampler)) { SourceLocation ElementLoc = getTokenLocation(CurToken); auto Sampler = parseStaticSampler(); if (!Sampler) { HadError = true; skipUntilExpectedToken(RootElementKeywords); continue; } Elements.emplace_back(RootSignatureElement(ElementLoc, *Sampler)); } else { consumeNextToken(); // let diagnostic be at the start of invalid token reportDiag(diag::err_hlsl_invalid_token) << /*parameter=*/0 << /*param of*/ TokenKind::kw_RootSignature; HadError = true; skipUntilExpectedToken(RootElementKeywords); continue; } if (!tryConsumeExpectedToken(TokenKind::pu_comma)) { // ',' denotes another element, otherwise, expected to be at end of stream break; } } return HadError || consumeExpectedToken(TokenKind::end_of_stream, diag::err_expected_either, TokenKind::pu_comma);

another somewhat silly idea could be a macro + a switch statement

#define PARSE_PART(parse_fn) \ consumeToken(); \ SourceLocation ElementLoc = getTokenLocation(CurToken); \ auto Part = parse_fn(); \ if (!Part) { \ HadError = true; \ skipUntilExpectedToken(RootElementKeywords); \ continue; \ } \ Elements.emplace_back(RootSignatureElement(ElementLoc, *Part)); \ while (!peekExpectedToken(TokenKind::end_of_stream)) { switch (peakToken()) { case TokenKind::kw_RootFlags: { PARSE_PART(parseRootFlags); } case TokenKind::kw_RootFlags: { PARSE_PART(parseRootFlags); } case TokenKind::kw_DescriptorTable: { PARSE_PART(parseDescriptorTable); } case TokenKind::kw_SRV: { PARSE_PART(parseRootDescriptor); } case TokenKind::kw_UAV: { PARSE_PART(parseRootDescriptor); } default: { consumeNextToken(); // let diagnostic be at the start of invalid token reportDiag(diag::err_hlsl_invalid_token) << /*parameter=*/0 << /*param of*/ TokenKind::kw_RootSignature; HadError = true; skipUntilExpectedToken(RootElementKeywords); } } #undef PARSE_PART }

I won't apply it here, but I will keep that in mind for the aforementioned clean-up pr. Thanks!

edit: the macro part, I applied the other suggestions

…eParser`

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" HLSL HLSL Language Support labels Jul 9, 2025

inbelic force-pushed the inbelic/rs-mult-errs branch from 89240be to 7ec7e32 Compare July 9, 2025 22:04

joaosaffran approved these changes Jul 11, 2025

View reviewed changes

spall reviewed Jul 11, 2025

View reviewed changes

V-FEXrt approved these changes Jul 11, 2025

View reviewed changes

inbelic added 3 commits July 12, 2025 02:47

[HLSL][RootSignature] Implement multiple diagnostics in `RootSignatur…

02e7ad8

…eParser`

review: correctly exit the table scope before reporting more errors

f589f36

nfc, review: update loop logic to remove use of local bool

39e450b

inbelic changed the base branch from users/inbelic/pr-147800 to main July 12, 2025 02:51

inbelic force-pushed the inbelic/rs-mult-errs branch from d54b222 to 39e450b Compare July 12, 2025 02:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[HLSL][RootSignature] Allow for multiple parsing errors in `RootSignatureParser` #147832

[HLSL][RootSignature] Allow for multiple parsing errors in `RootSignatureParser` #147832

inbelic commented Jul 9, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jul 9, 2025 •

edited

Loading

Uh oh!

spall Jul 11, 2025

Uh oh!

inbelic Jul 11, 2025

Uh oh!

Uh oh!

Uh oh!

V-FEXrt Jul 11, 2025

Uh oh!

inbelic Jul 11, 2025 •

edited

Loading

Uh oh!

Uh oh!

[HLSL][RootSignature] Allow for multiple parsing errors in RootSignatureParser #147832

Are you sure you want to change the base?

[HLSL][RootSignature] Allow for multiple parsing errors in RootSignatureParser #147832

Conversation

inbelic commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spall Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

inbelic Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

V-FEXrt Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

inbelic Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[HLSL][RootSignature] Allow for multiple parsing errors in `RootSignatureParser` #147832

[HLSL][RootSignature] Allow for multiple parsing errors in `RootSignatureParser` #147832

inbelic commented Jul 9, 2025 •

edited

Loading

llvmbot commented Jul 9, 2025 •

edited

Loading

inbelic Jul 11, 2025 •

edited

Loading